Fix: Test memory leak and Failing test when md5 and sha are enabled - #11025
Merged
dgarske merged 4 commits intoAug 6, 2026
Merged
Conversation
|
Frauschi
reviewed
Jul 31, 2026
Frauschi
left a comment
Frauschi
left a comment
Contributor
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 6 total — 2 posted, 4 skipped
Posted findings
- [Medium] Weak-hash guard is skipped even when WC_SIG_MIN_HASH_TYPE raises the floor, and silently self-destructs if the default ever moves into a header —
tests/api/test_signature.c:338 - [Medium] Lowered-floor branch of wc_SignatureCheckHashStrength() has no coverage at all —
tests/api/test_signature.c:338
Skipped findings
- [Low] PR description attributes the failure to --enable-sha/--enable-md5, but the trigger is --enable-wolfclu
- [Low] Two different init/free-pairing conventions now coexist in this file
- [Low] Weak-hash assertions skipped for any WC_SIG_MIN_HASH_TYPE override, including stronger floors
- [Info] Guard depends on WC_SIG_MIN_HASH_TYPE's default living in signature.c, not a public header
Review generated by Skoll via Claude/Codex
Member
Author
|
Retest this please |
Frauschi
reviewed
Aug 3, 2026
Frauschi
left a comment
Frauschi
left a comment
Contributor
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 2 total — 2 posted, 0 skipped
Posted findings
- [Medium] Lowered-floor else branch at the generate site calls wc_SignatureVerify() instead of wc_SignatureGenerate() —
tests/api/test_signature.c:538-544 - [Low] Guard compares wc_HashType enum values; the library compares digest sizes, and the two disagree in real configs —
tests/api/test_signature.c:345,532
Review generated by Skoll via Claude/Codex
aidankeefe2022
force-pushed
the
test-fix-for-md5-and-sha-enabled
branch
from
August 4, 2026 21:59
6ae7980 to
41c6766
Compare
Member
Author
|
Fix for failing tests was resolved by #11026 but fix for memory leak on failure is real and DoExpect for rng init should still go in |
… not leak memory on failure
aidankeefe2022
force-pushed
the
test-fix-for-md5-and-sha-enabled
branch
from
August 4, 2026 22:01
41c6766 to
2f9fc44
Compare
aidankeefe2022
assigned
Frauschi,
wolfSSL-Bot and
aidankeefe2022 and unassigned
aidankeefe2022,
Frauschi and
wolfSSL-Bot
Contributor
|
Jenkins retest this please |
Frauschi
reviewed
Aug 5, 2026
Frauschi
left a comment
Frauschi
left a comment
Contributor
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 3 total — 1 posted, 2 skipped
Posted findings
- [Medium] RNG fix deviates from the file's own (and the repo's dominant) zero-then-init idiom —
tests/api/test_signature.c:307
Skipped findings
- [Low] PR description no longer matches the diff: the WC_SIG_MIN_HASH_TYPE half already landed on master
- [Info] Fix is unverified by execution in this review environment
Review generated by Skoll via Claude/Codex
Member
Author
|
retest this please |
dgarske
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixed a memory leak where if _ret is already failing, the rngInit function will not run, causing rngFree to free garbage data. The fix was to have rngInit always run to match with rngFree by swapping ExpectInt to DoExpectInt. Fixed failing test where WC_SIG_MIN_HASH_TYPE was not properly set for --enable-sha and --enable-md5, so we skip under those conditions.
Found while testing wolfCLU
Testing
Code no longer leaks, and code no longer fails with the wrong code for the test when SHA and MD5 are enabled